Skip to content

fix: fix selecting parent of unique nodes#1430

Merged
devsergiy merged 3 commits intomasterfrom
sergiy/eng-9097-fix-wrong-external-parent-selection-when-use-interfaces
Mar 6, 2026
Merged

fix: fix selecting parent of unique nodes#1430
devsergiy merged 3 commits intomasterfrom
sergiy/eng-9097-fix-wrong-external-parent-selection-when-use-interfaces

Conversation

@devsergiy
Copy link
Copy Markdown
Member

@devsergiy devsergiy commented Mar 5, 2026

Summary by CodeRabbit

  • Tests

    • Enhanced federation test coverage with comprehensive multi-subgraph scenario testing, including abstract types and cross-subgraph field resolution patterns.
  • Bug Fixes

    • Improved parent node selection logic in federated queries to prevent unnecessary traversal in certain external field scenarios.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 5, 2026

📝 Walkthrough

Walkthrough

The PR adds a comprehensive federation test case demonstrating unique nodes parent root node selection with a multi-subgraph "zigzag" resolution pattern. It also modifies the parent traversal logic in datasource_filter_visitor to break at external, non-provided parents instead of continuing upward.

Changes

Cohort / File(s) Summary
Federation Test Addition
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
Adds substantial subtest "unique nodes parent root nodes selection" demonstrating 2-subgraph zigzag resolution with Node interface, distributed Pool/Detail artifacts, and entity field resolution across subgraph boundaries.
Parent Traversal Logic
v2/pkg/engine/plan/datasource_filter_visitor.go
Modifies selectUniqNodeParentsUpToRootNode to break traversal when encountering external, non-provided parents rather than continuing upward, tightening parent selection semantics.

Possibly related PRs

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: fix selecting parent of unique nodes' directly addresses the core issue fixed in the PR by correcting parent selection for unique nodes in federation scenarios.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sergiy/eng-9097-fix-wrong-external-parent-selection-when-use-interfaces

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

@devsergiy devsergiy changed the title Sergiy/eng 9097 fix wrong external parent selection when use interfaces fix: fix wrong external parent selection when use interfaces Mar 5, 2026
@devsergiy devsergiy changed the title fix: fix wrong external parent selection when use interfaces fix: fix selecting parent of unique nodes Mar 5, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)

20223-20245: Test narrative is stale and contradicts the actual scenario.

These comments describe NodeB/Pool/Pool1/sharedField and second-subgraph ownership of uniqueField, but the test actually models NodeA -> Detail and resolves uniqueField from the first subgraph. Please align comments with the implemented schema and expected fetches.

♻️ Suggested comment cleanup
-		// Tests a scenario where the planner must navigate through multiple layers of
-		// abstract types with `@external` fields to reach an entity whose fields are
-		// split across subgraphs. Mirrors a real-world pattern where:
-		//
-		//   - Node (interface) is implemented by NodeA and NodeB
-		//   - NodeA/NodeB declare pool: Pool! as `@external` in the first subgraph
-		//     (to satisfy the Node interface), but Pool is truly owned by the second subgraph
-		//   - Pool (interface) is implemented by Pool1
-		//   - Pool1.detail: Detail! is `@external` in the first subgraph, owned by the second
-		//   - Detail is an entity with fields split across subgraphs:
-		//       uniqueField → second subgraph only
-		//       sharedField → first subgraph only
-		//
-		// Expected execution (zigzag: first → second → first):
-		//   Fetch 0: first  subgraph — root nodes query, returns NodeA keys
-		//   Fetch 1: second subgraph — entity-resolve NodeA, traverse pool → Pool1 → detail,
-		//                              inline Detail.uniqueField (owned by second subgraph)
-		//   Fetch 2: first  subgraph — entity-resolve Detail to get sharedField
-		//
-		// Planner confusion: Pool1 appears as an entity in the first subgraph with
-		// detail `@external`. The planner may incorrectly try to resolve detail through
-		// the first subgraph's Pool1 entity, not realising it must go via the second
-		// subgraph's NodeA entity resolution first.
+		// Regression: ensure parent selection does not stop at an external interface field.
+		// Scenario:
+		//   - `Query.nodes` is fetched from first subgraph.
+		//   - `NodeA.detail` is external in first subgraph and resolved via second subgraph.
+		//   - `Detail.uniqueField` is then resolved back in first subgraph via entity jump.
+		//
+		// Expected execution: first -> second -> first.
@@
-			// Second subgraph: owns pool on NodeA/NodeB, owns detail on Pool1,
-			// owns Detail.uniqueField. Detail.sharedField lives only in first subgraph.
+			// Second subgraph: resolves NodeA.detail and returns Detail keys.
@@
-								// Fetch 1: entity-resolve NodeA from second subgraph.
-								// Second subgraph owns pool (non-external on NodeA), Pool1.detail, and Detail.uniqueField,
-								// so all three can be resolved inline without additional entity fetches.
+								// Fetch 1: entity-resolve NodeA in second subgraph and return Detail.id.
@@
-								// Fetch 2: entity-resolve Detail from first subgraph to get sharedField.
-								// This is the jump back — first subgraph is the only owner of Detail.sharedField.
+								// Fetch 2: entity-resolve Detail in first subgraph to get uniqueField.

Also applies to: 20359-20360, 20475-20477, 20516-20517

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go`
around lines 20223 - 20245, The test comments for the federation scenario are
stale and misdescribe the implemented schema and expected fetch sequence; update
the narrative comments that mention NodeB/Pool/Pool1/sharedField and the
ownership of uniqueField so they accurately reflect the actual test
implementation (which models NodeA -> Detail and resolves uniqueField from the
first subgraph), and adjust the listed expected execution steps to match the
real fetch order and ownership (references: Node, NodeA, NodeB, Pool, Pool1,
Detail, uniqueField, sharedField and the “zigzag” fetch sequence). Ensure you
update all occurrences noted (around the blocks near the ones flagged: the
current comment block and the later blocks at the same pattern) so comments
align with the schema and expected fetches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go`:
- Around line 20223-20245: The test comments for the federation scenario are
stale and misdescribe the implemented schema and expected fetch sequence; update
the narrative comments that mention NodeB/Pool/Pool1/sharedField and the
ownership of uniqueField so they accurately reflect the actual test
implementation (which models NodeA -> Detail and resolves uniqueField from the
first subgraph), and adjust the listed expected execution steps to match the
real fetch order and ownership (references: Node, NodeA, NodeB, Pool, Pool1,
Detail, uniqueField, sharedField and the “zigzag” fetch sequence). Ensure you
update all occurrences noted (around the blocks near the ones flagged: the
current comment block and the later blocks at the same pattern) so comments
align with the schema and expected fetches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6be3fa7d-4684-4776-858c-2ee5eaec7da5

📥 Commits

Reviewing files that changed from the base of the PR and between 0bab722 and 7267001.

📒 Files selected for processing (2)
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
  • v2/pkg/engine/plan/datasource_filter_visitor.go

@devsergiy devsergiy merged commit fd0e06a into master Mar 6, 2026
10 checks passed
@devsergiy devsergiy deleted the sergiy/eng-9097-fix-wrong-external-parent-selection-when-use-interfaces branch March 6, 2026 00:32
devsergiy pushed a commit that referenced this pull request Mar 6, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.0.0-rc.261](v2.0.0-rc.260...v2.0.0-rc.261)
(2026-03-06)


### Bug Fixes

* fix selecting parent of unique nodes
([#1430](#1430))
([fd0e06a](fd0e06a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
  * Fixed an issue with selecting parent of unique nodes

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant